-
Notifications
You must be signed in to change notification settings - Fork 47.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added production profiling bundle type #12886
Conversation
|
|
Details of bundled changes.Comparing: bfb12eb...cd39422 react-dom
react-native-renderer
Generated by 🚫 dangerJS |
a50b70c
to
812beec
Compare
Rebaaaaased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
At some point, I'd like to figure out how to do a limited build set to speed up turn around time when testing DOM fixtures. We only need the UMD dev (edit: dom and core) builds in that case.
yarn build "dom-client, core"
definitely speeds things up, and some times I just comment out the other build 😸.
Still at some point I'd like to look into adding a more fine grained filter (also maybe running Rollup in watch mode).
Thanks Nate! 😄 Have you tried? yarn build dom-client,core --type=UMD |
Any reviewers other than Nate want to take a look and weigh in? |
packages/shared/ReactFeatureFlags.js
Outdated
@@ -37,7 +37,8 @@ export const warnAboutDeprecatedLifecycles = false; | |||
export const warnAboutLegacyContextAPI = false; | |||
|
|||
// Gather advanced timing metrics for Profiler subtrees. | |||
export const enableProfilerTimer = __DEV__; | |||
export const enableProfilerTimer = | |||
__DEV__ || process.env.REACT_ENABLE_PROFILING === true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to be on process.env, as opposed to another global?
My understanding is that this flag always gets replaced in build process. So we don’t actually let people use it, do we? Not unless they import our bundle directly. At that point we might as well call it __PROFILE__
to better indicate its internal only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case it’s not clear I mean that we didn’t change npm entry point so there’s no way external consumers can set this variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally named it __PROFILER__
but backed it out in 812beec because of Flow. I don't feel strongly about that change though. Happy to change it back and just add a Flow declaration.
Edit Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mostly curious about your intention. What is your plan for people to use this in CJS mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intention wasn't to let people override this value at runtime, so you're right that using process.env
was probably misleading or confusing.
What is your plan for people to use this in CJS mode?
Haven't thought a lot about this, but I was assuming people would alias react (and renderer) imports for production builds if they wanted to use profiling capabilities.
I think at some point we discussed allowing to opt into profiling bundle with an env variable (instead of aliasing). But I haven’t thought about whether it would break dead code elimination or not. |
My interpretation/recollection of that idea was that a query parameter and/or env variable could be used to determine which version of react (and renderer) was required- production or profiling. Including profiling capabilities in the production build would impact DCE for places like |
I didn’t propose to include it in the production build, but to do a switch of the bundle at the top level (just like we do with development/production). But I’m worried that there might be no way to have a separate env variable without breaking dead code elimination for people who don’t set it at all. |
I guess I misunderstood your initial suggestion. I don't know that we need to decide that aspect in order to move on with this PR. Even just having a production+profiling bundle that people could alias to themselves would be a nice incremental change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good.
Before merging can you please make sure once again this didn't affect existing DEV/PROD bundles now that you've changed it to use |
Do we want people aliasing though? We wanted to treat the nested bundles as opaque so that we can do things like inlining it into the main bundle to avoid packaging woes. Seems like mixed messaging. It's also not a very scalable solution because we might want to move things to the react package and later the scheduler package. All of those have to be aliased and things might break in weird ways when you get it wrong. A possible solution could be to embed something like |
That's additive though. We can still merge this for initial experimentation. Then when we announce |
Is there any concern that adding an environment variable could lead to component ecosystem around I don't think that decision should block this PR, but that's my only concern with using an environment variable. |
I don't see why that would be bad. I'm more concerned that DCE might not work at all for some people because they don't explicitly have this variable in their configs. |
Yup! I'll rebase and do one more comparison. |
Adds a new "profiling" target to ReactDOM (cjs) and ReactNative (oss) bundles. This is a production build (
process.env.NODE_ENV === "production"
) but has the profiling timing flag enabled (via a new env variableprocess.env.REACT_ENABLE_PROFILING
).I don't believe that other bundles need this new build type, but it would be easy enough to add if we decided it did.